BUG: add missing _check_type_device calls#103
Conversation
|
Should add some tests for this. |
|
I believe 6d67d46#diff-8047174d1f9158b619da8e809ed90fd46404caae67e7f687c5ba3a77f5bddc40R215 is a faithful representation of what we can test. |
|
Ah sorry, I didn't notice that last bit was actually already in the tests. That should be fine. This change seems OK. It fixes some of the more egregious uses of |
|
Indeed, scipy's W.r.t. CI testing of downstreams, yeah, this might be useful. An immediate downside though is that it'll make a CI run from under five minutes to > 1/2 hour. It is definitely worth thinking a bit about a larger CI strategy, here or in -compat. Maybe as a cron job or a manual trigger. I feel it should be a separate PR though. |
|
I would go with whatever's the most pragmatically useful for testing scipy. It would be a good idea to test it before doing releases at the very least, to avoid the issues we had previously. |
array_api_strict/_array_object.py
Outdated
| elif isinstance(other, Array): | ||
| if self.device != other.device: | ||
| raise ValueError(f"Arrays from two different devices ({self.device} and {other.device}) can not be combined.") | ||
| else: |
There was a problem hiding this comment.
Maybe we should explicitly only reject ndarray. That way NotImplemented can still work on other types (although I honestly don't know how important that is).
There was a problem hiding this comment.
I'd think that mixing different array objects is a bad idea in general, and is basically impossible to make consistent. Even when it fails, there are just too many possible reasons or failure modes.
So one either buys in duck-typing all the way (and whatever quacking falls out, it does), or say no to anything which is not the same array namespace. If -strict strives to be strict, the latter makes sense, it seems.
That said, I'm happy with whatever you think is best.
In [4]: xp.arange(5) + cp.arange(5)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[4], line 1
----> 1 xp.arange(5) + cp.arange(5)
File cupy/_core/core.pyx:1265, in cupy._core.core._ndarray_base.__add__()
File cupy/_core/_kernel.pyx:1286, in cupy._core._kernel.ufunc.__call__()
File cupy/_core/_kernel.pyx:159, in cupy._core._kernel._preprocess_args()
File cupy/_core/_kernel.pyx:145, in cupy._core._kernel._preprocess_arg()
TypeError: Unsupported type <class 'array_api_strict._array_object.Array'>
In [5]: cp.arange(3) + xp.arange(3)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[5], line 1
----> 1 cp.arange(3) + xp.arange(3)
File cupy/_core/core.pyx:1269, in cupy._core.core._ndarray_base.__add__()
TypeError: operand type(s) all returned NotImplemented from __array_ufunc__(<ufunc 'add'>, '__call__', array([0, 1, 2]), Array([0, 1, 2], dtype=array_api_strict.int64)): 'ndarray', 'Array'
There was a problem hiding this comment.
So one either buys in duck-typing all the way (and whatever quacking falls out, it does), or say no to anything which is not the same array namespace. If -strict strives to be strict, the latter makes sense, it seems.
That sounds good to me.
ev-br
force-pushed
the
binops_reject_ndarrays
branch
2 times, most recently
from
e5450f6 to
5849136
Compare
ev-br
force-pushed
the
binops_reject_ndarrays
branch
from
5849136 to
8bc3de3
Compare
|
The main goal of this PR, to reject What this PR does now is, in effect, it adds |
ev-br
changed the title
_check_type_device calls
ev-br
changed the title
_check_type_device calls_check_type_device calls
3 participants
A minimal fix for gh-102.
As a byproduct, add several
check_devicecalls, where previously missing.